feat(exex): finalize WAL only when all ExExes are on the canonical chain #11289
Conversation
shekhirin
added
C-enhancement
shekhirin
changed the title
shekhirin
requested review from
mattsse,
Rjected,
onbjerg and
gakonst
as code owners
crates/exex/exex/src/manager.rs
Outdated
| debug!(header = ?header.num_hash(), "Received finalized header"); | ||
|
|
||
| // Check if all ExExes are on the canonical chain | ||
| let exex_finished_heights = this | ||
| .exex_handles | ||
| .iter() | ||
| // Get ExEx ID and hash of the finished height for each ExEx | ||
| .map(|exex_handle| { | ||
| (&exex_handle.id, exex_handle.finished_height.map(|block| block.hash)) | ||
| }) | ||
| // Deduplicate all hashes | ||
| .unique_by(|(_, hash)| *hash) | ||
| // Check if hashes are canonical | ||
| .map(|(exex_id, hash)| { | ||
| hash.map_or(Ok((exex_id, hash, false)), |hash| { | ||
| this.provider | ||
| .header(&hash) | ||
| // Save the ExEx ID, hash of the finished height, and whether the hash | ||
| // is canonical | ||
| .map(|header| (exex_id, Some(hash), header.is_some())) | ||
| }) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| if exex_finished_heights.iter().all(|(_, _, is_canonical)| *is_canonical) { | ||
| // If there is a finalized header and all ExExs are on the canonical chain, finalize |
There was a problem hiding this comment.
this is very hard to read now, this entire poll function is massive, can we please split this
There was a problem hiding this comment.
fair, moved to a separate fn
crates/exex/exex/src/manager.rs
Outdated
| .filter_map(|(exex_id, hash, is_canonical)| { | ||
| is_canonical.not().then_some((exex_id, hash)) | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
this collect is redundant, itertools has helper for this
There was a problem hiding this comment.
the collect is for debug logging below
reth/crates/exex/exex/src/manager.rs
Lines 369 to 378 in 03fcdb4
crates/exex/exex/src/manager.rs
Outdated
| .filter_map(|(exex_id, hash, is_canonical)| { | ||
| is_canonical.not().then_some((exex_id, hash)) | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
what I meant was this is redundant because only used for debug trace and we now have access to
https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format
| let this = self.get_mut(); | ||
|
|
||
| // Handle incoming ExEx events | ||
| for exex in &mut this.exex_handles { |
There was a problem hiding this comment.
do we actually spawn the manager if there are 0 exexs?
if we do, then we need a shortcut because otherwise we do unnecessary wal IO?
There was a problem hiding this comment.
we don't spawn it if there are no ExExes
Towards #11262
Calls
Wal::finalizeonly when all ExExes are on the canonical chain, according to the last emittedFinishedHeight, so we're sure that we will not delete any notifications that may be necessary for reverting to the canonical chain from a fork.